-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add forwarded-host-header and forwarded-prefix-header configuration #9809
Conversation
extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/ForwardedParser.java
Outdated
Show resolved
Hide resolved
extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/ForwardedParser.java
Show resolved
Hide resolved
...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/HttpConfiguration.java
Outdated
Show resolved
Hide resolved
...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/HttpConfiguration.java
Outdated
Show resolved
Hide resolved
...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java
Outdated
Show resolved
Hide resolved
@ejba IMHO this looks very neat, thanks for spending the time on this PR. Please consider addressing a few minor comments I've left and also remove |
It is important to mention that @kraeftbraeu was also a contributor. He helped me to accomplish this PR! Thanks @kraeftbraeu! |
extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/ForwardedParser.java
Outdated
Show resolved
Hide resolved
@ejba Thanks, and of course everyone has noticed the contribution from @kraeftbraeu, nice of you to highlight it |
I think it is green, only Java14 build fails to start for some reasons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartwdouglas Hi Stuart, can you please have a quick look as well in case we've missed something ? Does the logic around the forwarded host and server headers look ok to you ? thanks
...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/HttpConfiguration.java
Outdated
Show resolved
Hide resolved
extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/ForwardedParser.java
Outdated
Show resolved
Hide resolved
this.delegate = delegate; | ||
this.allowForward = allowForward; | ||
this.forwardedHostHeader = forwardedHostHeader; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must be optional, both of these need to have a specific configuration option to turn them on.
To give you an idea of why this can cause problems say I have an environment that knows nothing about X-Forwarded-Prefix, but security is done by the front end proxy.
An attacker could send a request to / but with X-Forwarded-Prefix set to /secure. The front end proxy will just see /, and allow it, but then Quarkus will serve up content from /secure.
If you are going to enable these options they can only be turned on in an environment where you know these headers will only be set by a trusted source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! :-)
For clarification, when you mentioned "both of these", do you mean x-forwarded-host/x-forwarded-server as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the "quarkus.http.proxy-address-forwarding" config property be enough to prevent those scenarios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I need to do some more research on this. The question is do frontend proxies that send one or more of these always send all of them (or make sure to disallow forwarding them)?
If all the main implementations do this then as this is not really a standard we are probably fine to just use a single switch. If not then maybe even our current configuration is too coarse grained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ejba Looks like a switch per header can be the most secure solution based on what you've discussed with @stuartwdouglas and @kraeftbraeu, please follow up with this update.
May be it can make sense to split the PR in 2 parts, the 1st part will only deal with X-Forwarded-Host
and X-Forwarded-Server
to accelerate it, X-Forwarded-Prefix
related updated can be discussed further as needed in another (draft) PR...
Sounds good ?
Cheers, Sergey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sberyozkin, just two questions:
- Should I keep this
quarkus.http.proxy-address-forwarding
property? - Should I default every x-forward-* header as disabled, despite the backward compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach with two separate PRs, since it helps separate needs. To be honest I still dont get the security issues with the prefix header. Isnt it set by the reverse proxy? Or is it possible that the reverse proxy just forwards these headers?
So, for the host/server selection two separate switches shall be used with a startup exception when both are set to true?
Or can we stay with the current solution but we extend the documentation (and maybe the startup log) with a hint to this security issue when using this option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property quarkus.http.proxy-address-forwarding
says if the absoluteUri shall be recalculated from forwarded-headers in principle. I'd say that should be kept.
When we start the additional properties
quarkus.http.allow-x-forwarded-server
quarkus.http.allow-x-forwarded-host
quarkus.http.allow-x-forwarded-prefix
and the second one is no more enabled by default, backward compatibility is broken.
I'm new to the quarkus project but IMHO broken backward compatibility reduces trust to a software project and should definitely be avoided if possible.
Of course security issues are still important. But in the end the key is the awareness by the quarkus users. And I would prefer to write a good documentation rather than making the configuration a little more complicated to push the user to think about security issues.
@stuartwdouglas what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, #10138 is the new PR.
Dunno what's going on but CI won't run. It might be related to the PR name being initially WIP but even restarting CI manually doesn't work. @stuartwdouglas could you have another look at it as it's probably better to finish the review here then I'll create another PR with this commit to trigger CI again. |
@gsmet Did I something wrong? :/ Maybe I can trigger CI with a forced push. |
Closes #9622